Skip to content

fix(mcp): open the OAuth popup synchronously and bound the start request#5824

Merged
waleedlatif1 merged 4 commits into
stagingfrom
fix/mcp-oauth-popup-first
Jul 22, 2026
Merged

fix(mcp): open the OAuth popup synchronously and bound the start request#5824
waleedlatif1 merged 4 commits into
stagingfrom
fix/mcp-oauth-popup-first

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

Fixes the two reported OAuth UX failures (no popup after Add MCP; 'Reopen authorization window' pressing doing nothing):

  • Popup-first: window.open previously ran after awaiting /api/mcp/oauth/start, outside the browser's user-activation window, so the popup was silently blocked — worst on the add-server flow (two awaits deep). The hook now opens about:blank synchronously in the click (named per server, so a re-click focuses/reuses an existing authorization window), navigates it once the start returns, and closes it on failure/already_authorized. A genuinely blocked popup shows a clear toast and skips the request entirely.
  • Bounded start: /oauth/start had no client-side timeout, so a stalled start (the known transient body-stall) held the re-entrancy guard and the connecting label indefinitely — the button looked dead. The request is now bounded at 30s; on timeout the popup closes, the label resets to 'Connect with OAuth', and retry is immediately available.
  • The mutation no longer opens windows (returns authorizationUrl + state); popup lifecycle lives in one place.

Residual note: the add-server auto-start still sits after the create request, so an extremely slow create+probe can still exceed the activation window — but it now degrades to a clear 'Popup blocked' toast and a clean, clickable 'Connect with OAuth' state instead of a stuck label.

Type of Change

  • Bug fix (UX)

Testing

  • Hook tests updated + 2 new: popup opens before the start request resolves (popup-first ordering), and a blocked popup skips the request and leaves the row idle. 10/10 green; tsc + biome clean.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Two bugs behind 'pressed Connect/Reopen and nothing happened':
- window.open ran AFTER awaiting /oauth/start, outside the browser's user
  activation, so the popup was silently blocked (worst on the add-server flow).
  Popup-first now: open about:blank synchronously in the click (named window,
  so a re-click focuses/reuses an existing authorization window), navigate it
  once the start returns; close it on failure/already_authorized; clear toast
  when genuinely blocked (and skip the request entirely).
- /oauth/start had no client timeout, so a stalled start held the re-entrancy
  guard and the connecting label indefinitely. Bounded at 30s; on timeout the
  popup closes, the label resets, and retry is immediately available.
@vercel

vercel Bot commented Jul 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 21, 2026 11:52pm

Request Review

@cursor

cursor Bot commented Jul 21, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches OAuth popup lifecycle and connecting-state semantics; behavior changes on reopen/failure but scope is limited to MCP OAuth client hooks.

Overview
Fixes MCP OAuth flows where Connect or Reopen authorization appeared to do nothing because the browser blocked popups opened after /oauth/start finished.

Popup-first: useMcpOauthPopup now calls window.open('about:blank', …) synchronously on click, then navigates with authorizationUrl when the start mutation returns. Blocked popups show a toast and skip the start request. useStartMcpOauth only validates the URL and returns { authorizationUrl, state }—it no longer opens windows.

Reconnect / reopen: Opening the named window blanks any prior auth tab, so retireFlows(serverId) runs immediately; a failed reopen clears Connecting instead of keeping a stale in-flight flow.

Bounded start: /oauth/start uses a 30s abort (with AbortSignal.timeout fallback) so stalled requests release the re-entrancy guard and let the hook close the pre-opened popup and reset UI.

Reviewed by Cursor Bugbot for commit d3b754f. Configure here.

Comment thread apps/sim/hooks/mcp/use-mcp-oauth-popup.ts
@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR improves the MCP OAuth authorization flow. The main changes are:

  • Opens the authorization popup before starting the network request.
  • Reuses named authorization windows and cleans them up on failure.
  • Bounds the OAuth start request with a 30-second timeout.
  • Adds a timeout fallback for browsers without AbortSignal.timeout.
  • Adds tests for popup ordering and blocked popups.

Confidence Score: 5/5

This looks safe to merge.

  • The fallback popup result is checked before a pending flow is registered.
  • Failure paths close the blank popup and clear the connecting state.
  • The timeout fallback supports browsers without AbortSignal.timeout.
  • No blocking issues were found in the changed code.

Important Files Changed

Filename Overview
apps/sim/hooks/mcp/use-mcp-oauth-popup.ts Moves popup creation and lifecycle management into the OAuth orchestration hook.
apps/sim/hooks/queries/mcp.ts Returns authorization metadata and adds a cross-browser timeout to the OAuth start request.
apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx Updates OAuth result mocks and tests popup-first and blocked-popup behavior.

Reviews (3): Last reviewed commit: "fix(mcp): close the blank popup when pos..." | Re-trigger Greptile

Comment thread apps/sim/hooks/mcp/use-mcp-oauth-popup.ts
Comment thread apps/sim/hooks/mcp/use-mcp-oauth-popup.ts Outdated
Comment thread apps/sim/hooks/queries/mcp.ts Outdated
- Retire prior flows as soon as the named popup opens (the open already blanked
  any prior auth window; a failed start must not leave a windowless flow
  'connecting' for the 10-minute safety timeout).
- Check the COOP-fallback window.open result; when blocked, clear the state and
  toast instead of registering a windowless pending flow.
- Feature-detect AbortSignal.timeout (Safari <16) with an AbortController
  fallback for the bounded /oauth/start.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/hooks/mcp/use-mcp-oauth-popup.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit d3b754f. Configure here.

@waleedlatif1
waleedlatif1 merged commit e51a867 into staging Jul 22, 2026
18 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/mcp-oauth-popup-first branch July 22, 2026 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant